Repository navigation
Fix several ScrollConsoleScreenBuffer ConPTY issues - #20774
Conversation
This comment has been minimized.
This comment has been minimized.
Dustin L. Howett (DHowett)
left a comment
There was a problem hiding this comment.
this seems right to me, thanks for like seven bug fixes :D
| // Several applications scroll the entire screen vertically with a whitespace fill, to essentially replicate | ||
| // what SU/SD can do. Our VT application is faster and better, so let's use that whenever applicable. |
There was a problem hiding this comment.
James Holderness (@j4james) does this seem reasonable to you? Win32 API ScrollConsole with a full-screen rectangle, moved only vertically, with no non-color attributes, being translated into SU or SD?
(My only question is what color it's filled with when we do this vs the console API?)
| // Several applications scroll the entire screen vertically with a whitespace fill, to essentially replicate | ||
| // what SU/SD can do. Our VT application is faster and better, so let's use that whenever applicable. |
There was a problem hiding this comment.
Personally I'd prefer we didn't use SU and SD for this, because this is not the standard way these operations are supposed to work, so the behavior is not reliable. The correct way to scroll buffer contents like this would be with DL and IL sequences, although the downside is that you'd need to home the cursor first.
There was a problem hiding this comment.
In what way SU/SD are not reliable in this context?
There was a problem hiding this comment.
(I cleared my approval to allow for the discussion - thx!)
There was a problem hiding this comment.
SU and SD are officially panning operations, for use when you have a page size that is larger than your visible window (see the VT510 documentation for SU and SD). When the page size is the same as the visible window they do nothing.
In addition to that, terminals that follow an Xterm-like interpretation of SU and SD don't all implement it the same way. Some (like Windows Terminal) will delete the top row when scrolling the full window up with SU. Others (like Xterm) will move the top row into the backscroll buffer.
And then of course there are terminals that don't implement SU or SD at all, simply because they're not a conformance requirement (they're defined as part of the windowing extension, so if you don't support windowing you wouldn't expect to implement them).
IL and DL are a level 1 conformance requirement, though, so they're much more likely to be supported consistently by modern terminals. Although if you want to be ultra-conservative, it's worth noting that they weren't implemented on the original VT100 range (they were only made a level 1 requirement later), so there may still be some strict VT100 emulators that won't support them either.
There was a problem hiding this comment.
Alright, I hope this is correct. A big chunk of code moved upward in there so I can early-return on empty scroll rects. I used DECSC/DECRC again to implement cursor and attribute restore, which... I guess is fine considering how much we (I?) came to rely on it.
There was a problem hiding this comment.
It just occurred to me that this IL/DL approach might have another little problem. If the cursor is visible at the time of the operation, it's possible the user will see it jumping to the home position and back again whenever a scroll operation occurs. Do we perhaps also need to hide and restore the cursor here?
There was a problem hiding this comment.
I'm alright with that in this case as we have other places in this flow where we save/restore the cursor and let it potentially flicker a little bit; if we tackle it for IL/DL we can do it all at once for the other ones too :)
| { | ||
| const auto clipViewport = clip ? Viewport::FromInclusive(*clip).Clamp(bufferSize) : bufferSize; | ||
| const auto sourceViewport = Viewport::FromInclusive(source); | ||
| const auto fillViewport = sourceViewport.Clamp(clipViewport); |
There was a problem hiding this comment.
This diff is certainly a lot easier to read with
git diff --diff-algorithm=histogram --color-moved=dimmed-zebra --color-moved-ws=allow-indentation-change main..dev/lhecker/20772-deccra-fixes
|
L, I fixed the PR body to not mention SU/SD |
Dustin L. Howett (DHowett)
left a comment
There was a problem hiding this comment.
I gave this a quick test in Terminal, PuTTY and WezTerm and am fairly comfortable with it. Thank you. and thank you James for us not stumbling into SU being ~ ~ unreliable ~ ~
- Fixed #20772 by fully committing to lazy DA1 capabilities path via `VtIo::SetDeviceAttributes`. It accidentally only did so with enabled cursor inheritance before. - Fixed #20773 by fixing the source/destination rectangle alignment in the non-DECCRA path of `ScrollConsoleScreenBuffer`. Also closes MSFT-64450466. As a drive-by: - Fixed the DECCRA path not restoring cursor attributes in conhost. - Reduced memory use in the non-DECCRA path. - Added a `IL/DL` sequence fast-pass for `ScrollConsoleScreenBuffer`. - Fixed a broken bounds checks in `Read/WriteConsoleOutputWImplHelper`. - Fixed the TraceLogging definition for `SMALL_RECT`. (cherry picked from commit 5951f69) Service-Card-Id: PVTI_lADOAF3p4s4BlIihzg--Oqg PVTI_lADOAF3p4s4BlIihzg--OrY Service-Version: 1.26
- Fixed #20772 by fully committing to lazy DA1 capabilities path via `VtIo::SetDeviceAttributes`. It accidentally only did so with enabled cursor inheritance before. - Fixed #20773 by fixing the source/destination rectangle alignment in the non-DECCRA path of `ScrollConsoleScreenBuffer`. Also closes MSFT-64450466. As a drive-by: - Fixed the DECCRA path not restoring cursor attributes in conhost. - Reduced memory use in the non-DECCRA path. - Added a `IL/DL` sequence fast-pass for `ScrollConsoleScreenBuffer`. - Fixed a broken bounds checks in `Read/WriteConsoleOutputWImplHelper`. - Fixed the TraceLogging definition for `SMALL_RECT`. (cherry picked from commit 5951f69) Service-Card-Id: PVTI_lADOAF3p4s4BQX0-zg--Omw PVTI_lADOAF3p4s4BQX0-zg--Op8 PVTI_lADOAF3p4s4BQX0-zg-_abM Service-Version: 1.25
VtIo::SetDeviceAttributes. It accidentally only did so with enabledcursor inheritance before.
in the non-DECCRA path of
ScrollConsoleScreenBuffer.Also closes MSFT-64450466.
As a drive-by:
IL/DLsequence fast-pass forScrollConsoleScreenBuffer.Read/WriteConsoleOutputWImplHelper.SMALL_RECT.